-
Notifications
You must be signed in to change notification settings - Fork 129
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Interpolate test colors between red and green #1827
base: master
Are you sure you want to change the base?
Conversation
Interpolating RGB colors is a bad idea in itself, but this doesn't even account for gamma. See https://www.alanzucconi.com/2016/01/06/colour-interpolation/ |
This comment is over-aggressive, if you ask me.
It is really a joy to propose improvements in this community, where the most common feedback is contempt... |
Sopel's comment could have been better formulated but then answering with
is not a nice response either. It would be nice if you included some images in your pr to highlight the visual changes, though. |
Sorry, didn't mean it this way, maybe it came like that because I tried to be concise. I wanted to say that the current state it will be pretty eye jarring when interpolating colors that are very far, like green with red, and the interpolated colors will have little to no value for the viewer. It's also unclear how it will play with the fixed font color. |
There is always the posibility of testing UX patches on DEV enviroment, Even on production much worse ideas and suggestions was either updated on production to gather feedback without merging or with merging and reverting, we welcome experimenting changes via PR's. |
You know, bulding fishtest from scratch and connecting workers to it is a time/energy-consuming task. |
sure, but if a pr includes an "example" it's a) much easier for everyone to view and judge initially (ofc having a real example page is much better and realistic) b) who knows when dev will be tested with this... |
As I said, to test in place one needs to install ubuntu and update system and build fishtest from scratch and connect workers and then see, I don't expect devs to be responsible of doing that without an easy and accessable enviroment to all rather than locally. For "b) who knows when dev will be tested with this..." |
Let me say that personally I dislike this, although I see no hard objections against it. |
RGB interpolation is not the best thing to do. E.g. https://www.alanzucconi.com/2016/01/06/colour-interpolation/ |
I think you linked the same link sopel linked, and I think that we should not jump to conclusions before actually seeing it
|
The solution here is to interpolate the color of the 0 LLR, that is over the gray line in a 3d color space for either the light or dark theme, with the saturated Green or Red of the finished test. To be honest a simple RGB interpolation should work mostly fine if done with the gray line. |
def color_run(run, WLD):
color = ""
sprt = run["args"].get("sprt")
if sprt:
llr = float(sprt["llr"])
low = float(sprt["lower_bound"])
up = float(sprt["upper_bound"])
percent = 2 * max(0, min(1, (llr - low) / (up - low))) - 1
# rejection color
# yellow (#FFFF00) or red (#FF6A6A)
color_low = "#FFFF00" if WLD[0] > WLD[1] else "#FF6A6A"
# acceptation color
# blue (#66CCFF) or green (#44EB44)
color_up = (
"#66CCFF"
if (float(sprt["elo0"]) + float(sprt["elo1"])) < 0
else "#44EB44"
)
color = color_low if percent < 0 else color_up
# calculate interpolated color
color = interpolate_color("#F5F5F5", color, abs(percent))
return color |
Are you puzzled because that test should not be yellow or because it is not the best scale of yellow? If it is for the latter reason: RGB interpolation is simple and fast but does not guarantee hue. One of my points was that the right way to interpolate is between the gray level of 0 LLR and the color of finished test, changing "saturation" in cylindrical coordinates or "chroma" in cone bicone coordinates. Another it was that CIELab is theoretical correct but useful only if the user is viewing with a monitor calibrated (luminance, white point, gamma) and profiled (icc), with an OS/application using a CMS, and this only if viewing a simple image with few color patches with the same hue. However, this PR works only with light theme, and this could be the real stopper. |
I am surprised that the LLR of the yellow test is between the LLRs of reddish tests. This is counter intuitive. |
Tertium datur :) If that Red/Yellow condition is correct, I suppose that the LLR should be continuous when # rejection color
# yellow (#FFFF00) or red (#FF6A6A)
color_low = "#FFFF00" if WLD[0] > WLD[1] else "#FF6A6A" |
Ok I see. Personally I think that yellow has no meaning in general. But it should definitely not be applied to a running SPRT test! |
Personally I dislike this patch. In the main page the tests are already sorted according to LLR. It's unnecessary IMO to add this cosmetic change. Also, it seems misleading to e.g. show a -2 LLR test as red, or a +2 LLR as green, as it's not meaningful to make predictions about the sprt based on the current LLR alone. |
closes #1827